Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add use_presto parameter to test_stats_by_project #121

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rtimmons
Copy link

Does what it says on the tin.

@rtimmons rtimmons requested review from a team and dbradf and removed request for a team May 11, 2022 21:58
@dbradf
Copy link
Contributor

dbradf commented May 12, 2022

Is this not controlled on the backend? It seems like making this an API option could be problematic.

  • Everything that uses the API will need to be updated if you ever want to get rid of the non-presto way of doing things.
  • Even after moving everything we know about (and there could be uses we are unaware of), there is nothing to stop new usages.
  • If something goes wrong with the presto version and we need to roll back, we now have to make changes to all the clients that have switched over.
  • This is not the only way to access the API. In particular, the new task generation code is written in rust, so it is making this call via a rust version of the API.

It seems like having some configuration on the backend that could switch this over might be easier to manage. It could switch over everything at once and if there are problems quickly switch everything back.

@zituo-jin
Copy link
Collaborator

@rtimmons is this PR still needed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants